Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: DH-18265 - correct bugs in replace() function #2

Merged

Conversation

lbooker42
Copy link
Contributor

@lbooker42 lbooker42 commented Jan 23, 2025

This PR corrects multiple bugs in replace()

  1. replace() returns true whether or not it does a replacement if there is a value in the current slot.
KeyedIntTestObject result;

// Standard Java Map behavior
final Map<Integer, KeyedIntTestObject> map = new HashMap<>();
result = map.putIfAbsent(0, new KeyedIntTestObject(0));
assertNull(result);
assertFalse(map.replace(0, new KeyedIntTestObject(10), new KeyedIntTestObject(0)));
assertTrue(map.replace(0, new KeyedIntTestObject(0), new KeyedIntTestObject(10)));
    
// Our map, under normal conditions
final KeyedIntTestObjectMap m = new KeyedIntTestObjectMap(10);
result = m.putIfAbsent(0, new KeyedIntTestObject(0));
assertNull(result);

// This replace should fail, since we do not match old values
assertFalse(m.replace(0, new KeyedIntTestObject(10), new KeyedIntTestObject(0)));
// This returns true, although the value was NOT updated
  1. replace() will replace a value even though the oldValue does not match under certain scenarios.
KeyedIntTestObject result;

final KeyedIntTestObjectMap m = new KeyedIntTestObjectMap(10);

// Setup the conditions for the bug to be triggered
final int capacity = m.capacity();

// This will hash to 0 internally
result = m.putIfAbsent(capacity, new KeyedIntTestObject(capacity));
assertNull(result);

// This will also initially hash to 0, but will be double hashed to an empty slot
result = m.putIfAbsent(0, new KeyedIntTestObject(0));
assertNull(result);

// Remove the first object, leaving a DELETED tombstone at the 0 slot
result = m.remove(capacity);
assertNotNull(result);
assertEquals(result.getId(), capacity);

// This replace should fail, since we do not match old values. However, it succeeds
// and the value is updated.
assertFalse(m.replace(0, new KeyedIntTestObject(10), new KeyedIntTestObject(0)));

@rcaudy rcaudy self-requested a review January 24, 2025 01:22
@rcaudy rcaudy added the bug Something isn't working label Jan 24, 2025
@lbooker42 lbooker42 changed the title fix!: DH-18265 fix: DH-18265 - correct bugs in replace() function Jan 24, 2025
@devinrsmith devinrsmith merged commit f07e05c into deephaven:main Jan 28, 2025
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants